Skip to content

Do not merge dead scope into break exit points when loop body always terminates#5541

Open
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-00szheh
Open

Do not merge dead scope into break exit points when loop body always terminates#5541
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-00szheh

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

When all paths in a loop body terminate (e.g. every branch of an if/else ends with break), NodeScopeResolver was merging the unreachable "dead" scope from getScope() into the post-loop scope. This caused stale variable types to leak into the result. For example, in a foreach (["a", "b", "c"] as $tag) where every branch sets $tag = null and breaks, PHPStan reported $tag as 'c'|null instead of null.

Changes

  • Unrolled constant-array foreach (tryProcessUnrolledConstantArrayForeach in src/Analyser/NodeScopeResolver.php):

    • Capture isAlwaysTerminating() before filterOutLoopExitPoints() to detect dead scopes
    • When body always terminated, set $iterEndScope to null (skip dead scope) and only use continue exit point scopes
    • Don't update $chainScope when no normal completion occurred for an iteration
    • When no iteration completed normally, build endScope from only break scopes instead of merging breaks into the pre-loop chainScope
  • Non-unrolled foreach (main foreach body processing):

    • Same pattern: capture isAlwaysTerminating() before filtering
    • Start $finalScope from null when body always terminated
    • Build scope from only continue and break exit points
  • While loop: capture isAlwaysTerminating() before filtering; when body always terminated, start break scope from null instead of dead $finalScope

  • Do-while loop: same fix — start break scope from null when body always terminated

  • For loop: same fix — start break scope from null when body always terminated

  • Updated test tests/PHPStan/Analyser/nsrt/for-loop-i-type.php: immediate break in for($i=1; $i<50; $i++) now correctly gives int<1, 49> instead of int<1, max> (the dead scope's $i >= 50 range no longer leaks)

Root cause

When all paths in a loop body terminate (break/return/throw), processStmtNodesInternal returns the scope after the always-terminating statement — a "dead" scope representing unreachable code. This scope still carries variable types from before the terminating statement. When break exit points were merged with this dead scope (via mergeWith), the stale types became a union with the break scope types, producing incorrect wider types.

The pattern affected all five loop constructs: foreach (both unrolled and non-unrolled paths), while, do-while, and for. The fix captures isAlwaysTerminating() before filterOutLoopExitPoints() (which strips the flag for break/continue-based termination) and uses it to exclude the dead scope from the final result.

Test

  • tests/PHPStan/Analyser/nsrt/bug-1946.php: comprehensive regression test covering:
    • Foreach with constant array where all if/else branches break with same type assignment (the reported bug)
    • Foreach with different break types across branches
    • Foreach where all branches return
    • Foreach with only-if break (no else)
    • Foreach with non-constant array where all branches break
    • Foreach with elseif/else all breaking
    • Foreach with mixed break/continue
    • While (true) with all-break body
    • While (rand) with all-break body (may not iterate)
    • Do-while with all-break body
    • For loop with all-break body

Fixes phpstan/phpstan#1946

…terminates

- When all paths in a loop body terminate (break/return/throw), the
  scope returned by getScope() is the unreachable "dead" scope after
  the always-terminating statement. This dead scope was being merged
  into the final post-loop scope, polluting it with stale types.
- Fix unrolled constant-array foreach: skip dead iterEndScope when
  body always terminated; when no iteration completed normally, build
  endScope from only break scopes instead of merging into chainScope.
- Fix non-unrolled foreach: when body always terminated, start
  finalScope from null and build it from only continue/break scopes.
- Fix while loop: capture isAlwaysTerminating before
  filterOutLoopExitPoints; when body always terminated, start break
  scope from null instead of dead finalScope.
- Fix do-while loop: same pattern — start break scope from null when
  body always terminated.
- Fix for loop: same pattern — start break scope from null when body
  always terminated.
- Update for-loop-i-type.php test: immediate break in for($i=1;$i<50)
  now correctly gives int<1,49> instead of int<1,max> since the dead
  scope's $i>=50 range no longer leaks through.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants